From 0cb6c85508ec54ed994ffe3df7aff9415c149695 Mon Sep 17 00:00:00 2001 From: Andrew Hamilton Date: Tue, 15 Sep 2026 17:49:10 -0500 Subject: [PATCH] [PATCH 2/3] term/serial: Reject caller-supplied port addresses under lockdown The serial command is available in lockdown mode and the port option accepts a raw port address or a raw mmio base address. This can be misused to allow an attacker to write to local memory and do things including disabling / bypassing secure boot as long as the console is available or an unsigned GRUB configuration is under the attacker's control. Update the serial logic to only accept a raw address in lockdown when a port is already registered at it, which means the platform described that UART: grub_ns8250_init() for the legacy com ports, grub_pciserial_init() for an enumerated PCI device, or an ACPI SPCR UART once "auto" has parsed the table. Names that are not addresses are untouched, so "auto", efi from grub_efiserial_init() - the route on arm64, which has neither ns8250 nor SPCR support - and lookups by registered name all keep working. Behavior without lockdown (no secure boot) is unchanged. Assisted-by: Claude:Opus-5 (Tweaked to fit into the 2.14 codebase by Steve McIntyre <93sam@debian.org> Signed-off-by: Andrew Hamilton Suggested-by: Benjamin Herrenschmidt Reviewed-by: Leo Sandoval Part-of: Gbp-Pq: Topic upstream Gbp-Pq: Name 0002-term-serial-Reject-caller-supplied-port-addresses-un.patch --- grub-core/term/ns8250-spcr.c | 6 +++-- grub-core/term/ns8250.c | 27 ++++++++++++++++--- grub-core/term/serial.c | 50 ++++++++++++++++++++++++++++++++---- include/grub/serial.h | 8 ++++-- 4 files changed, 78 insertions(+), 13 deletions(-) diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c index 428b2d5..454946d 100644 --- a/grub-core/term/ns8250-spcr.c +++ b/grub-core/term/ns8250-spcr.c @@ -83,11 +83,13 @@ grub_ns8250_spcr_init (void) switch (spcr->base_addr.space_id) { + /* The address comes from the platform's own ACPI table, so it is trusted. */ case GRUB_ACPI_GENADDR_MEM_SPACE: return grub_serial_ns8250_add_mmio (spcr->base_addr.addr, - spcr->base_addr.access_size, &config); + spcr->base_addr.access_size, &config, + true); case GRUB_ACPI_GENADDR_IO_SPACE: - return grub_serial_ns8250_add_port (spcr->base_addr.addr, &config); + return grub_serial_ns8250_add_port (spcr->base_addr.addr, &config, true); default: return NULL; }; diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c index 23e8e09..014bb04 100644 --- a/grub-core/term/ns8250.c +++ b/grub-core/term/ns8250.c @@ -347,7 +347,8 @@ grub_ns8250_hw_get_port (const unsigned int unit) } struct grub_serial_port * -grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config) +grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config, + bool trusted) { struct grub_serial_port *p; unsigned i; @@ -363,14 +364,23 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config return &com_ports[i]; } + /* + * use_mmio and port share a union with the private data of every other back + * end, so they only mean anything on a port this driver registered. + */ FOR_SERIAL_PORTS (p) - if (p->use_mmio == false && p->port == port) + if (p->driver == &grub_ns8250_driver + && p->use_mmio == false && p->port == port) { if (config != NULL) grub_serial_port_configure (p, config); return p; } + /* No port is registered here, so going on means probing the address. */ + if (grub_serial_reject_untrusted_address (trusted) == true) + return NULL; + grub_outb (0x5a, port + UART_SR); if (grub_inb (port + UART_SR) != 0x5a) return NULL; @@ -402,7 +412,7 @@ grub_serial_ns8250_add_port (grub_port_t port, struct grub_serial_config *config struct grub_serial_port * grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int acc_size, - struct grub_serial_config *config) + struct grub_serial_config *config, bool trusted) { struct grub_serial_port *p; unsigned i; @@ -415,14 +425,23 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int acc_size, return &com_ports[i]; } + /* + * use_mmio and mmio share a union with the private data of every other back + * end, so they only mean anything on a port this driver registered. + */ FOR_SERIAL_PORTS (p) - if (p->use_mmio == true && p->mmio.base == addr) + if (p->driver == &grub_ns8250_driver + && p->use_mmio == true && p->mmio.base == addr) { if (config != NULL) grub_serial_port_configure (p, config); return p; } + /* No port is registered here, so going on means programming the address. */ + if (grub_serial_reject_untrusted_address (trusted) == true) + return NULL; + p = grub_malloc (sizeof (*p)); if (p == NULL) return NULL; diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c index 8260dcb..57039dd 100644 --- a/grub-core/term/serial.c +++ b/grub-core/term/serial.c @@ -28,6 +28,7 @@ #include #include #include +#include #ifdef GRUB_MACHINE_MIPS_LOONGSON #include #endif @@ -140,6 +141,30 @@ static struct grub_term_output grub_serial_term_output = +/* + * An address that no port is registered at cannot be honored while lockdown + * is enforced: the probe and programming sequences in the ns8250 and pl011 + * back ends write to fixed offsets from it with no way to tell a UART from + * ordinary memory, so an address taken from an unauthenticated configuration + * is a write primitive against whatever it points at. + * + * A back end calls this once its own lookup has found no port already + * registered at the address, that is, at the point where it would otherwise + * touch the hardware. TRUSTED is true when the address came from a platform + * description -- grub_ns8250_init(), an ACPI SPCR table, PCI enumeration -- + * rather than from the caller. + */ +bool +grub_serial_reject_untrusted_address (bool trusted) +{ + if (trusted == true || grub_is_lockdown () != GRUB_LOCKDOWN_ENABLED) + return false; + + grub_error (GRUB_ERR_ACCESS_DENIED, + N_("serial port address is not permitted when lockdown is enforced")); + return true; +} + struct grub_serial_port * grub_serial_find (const char *name) { @@ -154,12 +179,18 @@ grub_serial_find (const char *name) if (grub_strcmp (port->name, name) == 0) return port; + /* + * Everything below this point builds a port from an address the caller + * gave us, so the back ends are told the address is untrusted: under + * lockdown they hand back a port only if one is already registered at that + * address, which means the platform described that UART. + */ #if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) && !defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC) if (grub_strncmp (name, "port", sizeof ("port") - 1) == 0 && grub_isxdigit (name [sizeof ("port") - 1])) { port = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof ("port") - 1], - 0, 16), NULL); + 0, 16), NULL, false); if (port != NULL) return port; } @@ -205,7 +236,7 @@ grub_serial_find (const char *name) grub_error (GRUB_ERR_BAD_ARGUMENT, N_("incorrect MMIO access size")); } - port = grub_serial_ns8250_add_mmio (addr, acc_size, NULL); + port = grub_serial_ns8250_add_mmio (addr, acc_size, NULL, false); if (port != NULL) return port; } @@ -275,9 +306,18 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, char **args) port = grub_serial_find (name); if (!port) - return grub_error (GRUB_ERR_BAD_ARGUMENT, - N_("serial port `%s' isn't found"), - name); + { + /* + * A back end that refused the port has already said why; keep that + * rather than reporting a misleading "isn't found". + */ + if (grub_errno == GRUB_ERR_ACCESS_DENIED) + return grub_errno; + + return grub_error (GRUB_ERR_BAD_ARGUMENT, + N_("serial port `%s' isn't found"), + name); + } config = port->config; diff --git a/include/grub/serial.h b/include/grub/serial.h index d7e0635..096209a 100644 --- a/include/grub/serial.h +++ b/include/grub/serial.h @@ -188,14 +188,18 @@ grub_serial_config_defaults (struct grub_serial_port *port) return port->driver->configure (port, &config); } +bool grub_serial_reject_untrusted_address (bool trusted); + #if defined(__mips__) || defined (__i386__) || defined (__x86_64__) void grub_ns8250_init (void); struct grub_serial_port *grub_ns8250_spcr_init (void); struct grub_serial_port *grub_serial_ns8250_add_port (grub_port_t port, - struct grub_serial_config *config); + struct grub_serial_config *config, + bool trusted); struct grub_serial_port *grub_serial_ns8250_add_mmio (grub_addr_t addr, unsigned int acc_size, - struct grub_serial_config *config); + struct grub_serial_config *config, + bool trusted); #endif #ifdef GRUB_MACHINE_IEEE1275 void grub_ofserial_init (void); -- 2.39.5